-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add BitFlip and Sometimes Emission classes #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
bool propose_clean(const std::vector<bool>& corrupted, | ||
std::mt19937* unused_prng) { | ||
return !corrupted[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't entirely understand how propose_clean
will be used, but would it be better to choose a random element of corrupted
to invert? Or is corrupted
expected to contain all the same values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrupted is expected to contain all the same values. Specifically, the contract for the corrupted vector is that it is the output of sample_corrupted over repeated calls with the same clean value.
cxx/emissions/sometimes.hh
Outdated
// We approximate the maximum likelihood estimate by taking the mode of | ||
// corrupted. The full solution would construct BaseEmissor and | ||
// BetaBernoulli instances for each choice of clean and picking the | ||
// clean with the lowest combined logp_score(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be "highest" instead of "lowest"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Fixed.
int max_count = 0; | ||
for (const SampleType& c: corrupted) { | ||
++counts[c]; | ||
if (counts[c] > max_count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional nit: it seems cleaner to me to do this after the loop terminates with std::max_element
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but the code to use std::max_element over a map turns out to be many lines that look ugly to me:
auto max = std::max_element(
std::begin(counts), std::end(counts),
[] (const auto &p1, const auto &p2) { return p1.second < p2.second; }
)
return max->first;
Sometimes() : bb(nullptr) {}; | ||
|
||
void incorporate(const std::pair<SampleType, SampleType>& x) { | ||
++(this->N); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding, how come N
is accessed by pointer here and not in the other Emission subclasses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really really wish I knew. I originally had it as ++N; and the compiler complained that no variable named N was defined in scope.
No description provided.